lsp: prevent out-of-range panic in autoimports/completions when symbol name or position is invalid (fixes #1691) #1756
+41
−11
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR adds bounds checking to prevent slice bounds out of range panics in the language server code. The changes add defensive programming measures to safely handle edge cases where positions or indices might be out of bounds when slicing strings or arrays.
- Added bounds checks before slicing source file text with position parameters
- Added validation for empty symbol names before accessing first character
- Added comprehensive bounds checking for node modules path slicing operations
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/ls/completions.go | Added bounds checks before slicing source file text in getWordLengthAndStart and getDotAccessor functions |
| internal/ls/autoimportsexportinfo.go | Added empty string check before accessing first character of symbol name |
| internal/ls/autoimports.go | Added comprehensive bounds checking for node modules path slicing operations and fixed missing closing brace |
arash-mosavi
changed the title
|
@microsoft-github-policy-service agree |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
iisaduan
reviewed
Sep 30, 2025
- Add bounds check before accessing names[0] when symbol names array may be empty - Add bounds checking in getNodeModuleRootSpecifier for component array access - Prevent panics when accessing array elements without verifying length Fixes microsoft#1691 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The exportInfoId counter was being read but never incremented, causing all export info entries to receive the same ID (1). When multiple goroutines called add() concurrently, they would all write to the same map entry (e.symbols[1]), creating a data race. This fix properly increments the counter after adding each entry, ensuring unique IDs and eliminating the race condition.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/ls/autoimports.go:1326
- Extra closing brace added that doesn't match any opening brace in the getNodeModuleRootSpecifier function. This will cause a compilation error.
return ""
}
|
|
||
| // Bounds check to prevent slice bounds out of range panic | ||
| fileName := moduleFile.FileName() | ||
| if topLevelPackageNameIndex+1 >= 0 && packageRootIndex >= 0 && topLevelPackageNameIndex+1 <= packageRootIndex && packageRootIndex <= len(fileName) { |
There was a problem hiding this comment.
[nitpick] The bounds checking condition is complex and hard to read. Consider extracting this logic into a helper function like isValidSliceRange(start, end, length int) bool to improve readability and maintainability.
Copilot uses AI. Check for mistakes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Prevent a slice out-of-range panic in the LSP when computing completions/auto-imports for edge-case positions or empty symbol names.
The change adds minimal bounds checks and early returns around string/slice indexing in the language server hot paths.
Fixes #1691.
Repro (one-liner)
Typing at boundary positions (e.g., before the first char, at
len(text), or on an empty/whitespace symbol) could lead to computing invalid indices and slicing out of range, causing a panic in completions/auto-imports.Root Cause
Helpers assumed valid non-empty symbols and in-range positions, then immediately sliced the source text or accessed
s[0]. For empty names or out-of-bounds positions, indices became negative or > len, triggering a panic.What’s changed (small & localized)
internal/ls/completions.gogetWordLengthAndStart/getDotAccessorbefore slicing.start >= endor symbol is empty.internal/ls/autoimports.gointernal/ls/autoimportsexportinfo.goChanges are narrowly scoped to the points where invalid indices were observed; no refactors or logic rewrites.
Behavior & Compatibility
Tests
-1,len(text),len(text)+1) and empty/whitespace symbols.Risk
Low. Defensive bounds checks and early returns only; no broader behavior changes.
Notes for maintainers
This PR comes from a fork; could you please Approve and run the workflows so required checks can report?
All local checks pass (
go test ./..., also with-race).